Normalize datetime types for unified query API by dai-chen · Pull Request #5408 · opensearch-project/sql

dai-chen · 2026-05-06T02:34:16Z

Description

Add postAnalysisRules to LanguageSpec.LanguageExtension and register DatetimeExtension in UnifiedPplSpec with two post-analysis rules:

DatetimeUdtNormalizeRule: Rewrites PPL datetime UDT return types (EXPR_DATE/TIME/TIMESTAMP) on RexCall nodes to standard Calcite types, enabling downstream consumers to process the plan without UDT-related failures.
DatetimeOutputCastRule: Adds a final LogicalProject that casts standard datetime output columns to VARCHAR, aligning with PPL's wire-format contract (ISO string representation).

Examples

Case 1: UDT Normalize Rule — source=events | eval d = DATE(name) | fields d

 Before:  LogicalProject(d=[DATE($1):EXPR_DATE])
 After:   LogicalProject(d=[DATE($1):DATE])

Case 2: Output Cast Rule — source=events | fields hire_date, start_time, created_at

 Before:  LogicalProject(hire_date=[$2], start_time=[$3], created_at=[$4])
            LogicalTableScan(table=[[events]])

 After:   LogicalProject(hire_date=[CAST($0):VARCHAR], start_time=[CAST($1):VARCHAR], created_at=[CAST($2):VARCHAR])
            LogicalProject(hire_date=[$2], start_time=[$3], created_at=[$4])
              LogicalTableScan(table=[[events]])

Notes

The UDT normalize rule rewrites datetime function return types to standard Calcite types, but a mismatch remains with the underlying PPL UDF implementations (which still operate on String values). This does not affect the Analytics Engine path (reimplements UDFs in DataFusion) but will fail on the UnifiedQueryCompiler path. Follow-up PR will address function implementations.
Currently, the output cast rule uses CAST(datetime AS VARCHAR) whose string format is engine-dependent: PPL Calcite produce ANSI SQL format (2024-01-15 12:00:00) like most other SQL databases (SparkSQL, PostgreSQL, MySQL, Oracle, SQL Server), while DataFusion produces ISO 8601 format (2024-01-15T12:00:00).

Related Issues

Part of #5250

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-05-06T02:36:11Z

PR Reviewer Guide 🔍

(Review updated until commit `af2b288`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [55]: Generate optimal physical plan to fully unify query execution and leverage Calcite's optimizer.
🔀 Multiple PR themes Sub-PR theme: Add postAnalysisRules extension point and OpenSearch datetime precision Relevant files: core/src/main/java/org/opensearch/sql/executor/OpenSearchTypeSystem.java api/src/main/java/org/opensearch/sql/api/spec/LanguageSpec.java Sub-PR theme: Implement DatetimeExtension with UDT normalize and output cast rules Relevant files: api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java api/src/main/java/org/opensearch/sql/api/spec/UnifiedPplSpec.java api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java
⚡ Recommended focus areas for review Stateful Singleton `DatetimeOutputCastRule.INSTANCE` is a singleton but `RelHomogeneousShuttle` may carry mutable traversal state. If the shuttle accumulates state across multiple `visit` calls (e.g., parent/child tracking), concurrent or sequential reuse of the singleton could produce incorrect results. Verify that `RelHomogeneousShuttle` is truly stateless before using a shared singleton. static final DatetimeOutputCastRule INSTANCE = new DatetimeOutputCastRule(); Stateful Singleton Same concern as `DatetimeOutputCastRule`: `DatetimeUdtNormalizeRule.INSTANCE` is a shared singleton of `RelHomogeneousShuttle`. If the base class holds any mutable traversal state, reusing the same instance across multiple `plan()` calls could cause subtle bugs. Confirm thread-safety and statelessness. static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule(); Missing Recursion `visit(RelNode other)` does not call `super.visit(other)` before processing, so child nodes are never visited. The cast wrapping is only applied to the root node passed in. If the plan has nested projections with datetime fields that are not at the root, they will not be cast. Compare with `DatetimeUdtNormalizeRule` which correctly calls `super.visit(other)` first. public RelNode visit(RelNode other) { List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } RexBuilder rexBuilder = other.getCluster().getRexBuilder(); List<RexNode> projects = new ArrayList<>(fields.size()); List<String> names = new ArrayList<>(fields.size()); // Cast datetime fields to VARCHAR for output; pass through others unchanged for (RelDataTypeField field : fields) { RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); RelDataType fieldType = field.getType(); if (isDatetimeType(fieldType.getSqlTypeName())) { projects.add(castToVarchar(rexBuilder, newField, fieldType)); } else { projects.add(newField); } names.add(field.getName()); } return LogicalProject.create(other, List.of(), projects, names); } UDT Nullable Mismatch `UdtMapping.fromUdtType` checks `instanceof AbstractExprRelDataType<?>` but does not handle the case where the UDT type is wrapped in a nullable type decorator. Depending on how Calcite wraps nullable types, the instanceof check may fail for nullable UDT columns, silently skipping normalization. static Optional<UdtMapping> fromUdtType(RelDataType type) { if (!(type instanceof AbstractExprRelDataType<?> e)) { return Optional.empty(); } ExprUDT udt = e.getUdt(); return Arrays.stream(values()).filter(u -> u.udtType == udt).findFirst(); } Rule Order Dependency Post-analysis rules are applied sequentially in registration order. `DatetimeUdtNormalizeRule` must run before `DatetimeOutputCastRule` because the cast rule checks for standard `SqlTypeName` datetime types (not UDTs). If the order is ever changed or a new extension registers rules in between, the output cast rule may miss UDT-typed columns. This ordering dependency is implicit and not enforced. for (var shuttle : context.getLangSpec().postAnalysisRules()) { plan = plan.accept(shuttle); } return plan;

github-actions · 2026-05-06T02:36:40Z

PR Code Suggestions ✨

Latest suggestions up to 58b0420

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent casting intermediate nodes' datetime fields The rule visits every `RelNode` in the tree but only wraps the root — it should only apply to the root node. As written, it will wrap datetime fields in intermediate nodes (e.g., a `LogicalFilter` or inner `LogicalProject`) with a `CAST(...):VARCHAR`, which would break subsequent operations that expect the original datetime type. The rule should first recurse into children (via `super.visit(other)`) and then only apply the cast projection at the top-level call site, or guard against wrapping non-root nodes. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53] @Override public RelNode visit(RelNode other) { + // Do NOT recurse — this rule is intended to wrap only the root node List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } - ... + + RexBuilder rexBuilder = other.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } return LogicalProject.create(other, List.of(), projects, names); } Suggestion importance[1-10]: 6 __ Why: The concern about wrapping intermediate nodes is valid in principle, but looking at the test cases (e.g., `testAllStandardDatetimeTypesCastToVarchar`), the rule appears to intentionally wrap all nodes with datetime fields. The `improved_code` is essentially the same as the existing code minus the `super.visit()` call, but the suggestion doesn't clearly demonstrate the fix. The behavior concern is real but the proposed fix may break the intended recursive traversal.	Low
Possible issue	Avoid reusing stateful shuttle singletons across plans `RelShuttle` implementations like `RelHomogeneousShuttle` are stateful — they may cache visited nodes internally. Reusing the same `INSTANCE` singleton across multiple calls to `plan()` can cause incorrect results or stale state being applied to a new plan. Each rule should be instantiated fresh per planning call rather than using shared singletons. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] +RelNode plan = strategy.plan(query); +for (var shuttle : context.getLangSpec().postAnalysisRules()) { + plan = plan.accept(shuttle); +} +return plan; - Suggestion importance[1-10]: 5 __ Why: The concern about stateful `RelShuttle` singletons is valid — `RelHomogeneousShuttle` uses a `visited` set internally that could cause issues across multiple planning calls. However, the `improved_code` is identical to the `existing_code`, making the suggestion incomplete as it doesn't show the actual fix (instantiating fresh shuttles per call).	Low
General	Assert all matching calls have correct type The helper only captures the first matching `RexCall` found (`ref.get() == null`), but after `DatetimeUdtNormalizeRule` rewrites the plan, the same operator name may appear multiple times (e.g., `DATE` appears twice in `testNestedUdfCallsNormalized`). The test for `DATE` in that case will silently validate only the first occurrence, potentially missing a non-normalized call. Consider asserting that all matching calls have the expected type, not just the first. api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [196-224] private static void assertCallType( RelNode plan, String operatorName, SqlTypeName expectedType, int expectedPrecision) { - AtomicReference<RexCall> ref = new AtomicReference<>(); + List<RexCall> found = new ArrayList<>(); plan.accept( new RelHomogeneousShuttle() { @Override public RelNode visit(RelNode other) { RelNode visited = super.visit(other); visited.accept( new RexShuttle() { @Override public RexNode visitCall(RexCall call) { - if (ref.get() == null - && call.getOperator().getName().equalsIgnoreCase(operatorName)) { - ref.set(call); + if (call.getOperator().getName().equalsIgnoreCase(operatorName)) { + found.add(call); } return super.visitCall(call); } }); return visited; } }); + assertFalse("No RexCall found for: " + operatorName, found.isEmpty()); + for (RexCall call : found) { + assertEquals(operatorName + " type", expectedType, call.getType().getSqlTypeName()); + if (expectedPrecision >= 0) { + assertEquals(operatorName + " precision", expectedPrecision, call.getType().getPrecision()); + } + } +} Suggestion importance[1-10]: 4 __ Why: The suggestion to validate all matching `RexCall` instances rather than just the first is a reasonable improvement to test thoroughness. However, this is a test quality improvement with limited impact on correctness of the production code.	Low

Previous suggestions

Suggestions up to commit af2b288

Category Suggestion Impact

Possible issue

Recursively visit child nodes before applying cast rule

The visit method does not recursively visit child nodes before applying the cast
rule. This means datetime fields in non-root nodes (e.g., sub-queries or nested
projections) will not be processed. The method should call super.visit(other) or
visitChildren(other) first to ensure the entire tree is traversed.

api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53]

 @Override
 public RelNode visit(RelNode other) {
-  List<RelDataTypeField> fields = other.getRowType().getFieldList();
+  RelNode visited = visitChildren(other);
+  List<RelDataTypeField> fields = visited.getRowType().getFieldList();
   if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) {
-    return other;
+    return visited;
   }
-  ...
-  return LogicalProject.create(other, List.of(), projects, names);
+
+  RexBuilder rexBuilder = visited.getCluster().getRexBuilder();
+  List<RexNode> projects = new ArrayList<>(fields.size());
+  List<String> names = new ArrayList<>(fields.size());
+
+  for (RelDataTypeField field : fields) {
+    RexNode newField = rexBuilder.makeInputRef(visited, field.getIndex());
+    RelDataType fieldType = field.getType();
+    if (isDatetimeType(fieldType.getSqlTypeName())) {
+      projects.add(castToVarchar(rexBuilder, newField, fieldType));
+    } else {
+      projects.add(newField);
+    }
+    names.add(field.getName());
+  }
+  return LogicalProject.create(visited, List.of(), projects, names);
 }

Suggestion importance[1-10]: 6

__

Why: The DatetimeOutputCastRule is designed as an output cast rule (applied to the root/top-level node), so not recursing into children may be intentional. However, if the rule is meant to wrap all datetime fields at every level, the missing visitChildren call could cause incomplete processing. The suggestion is valid but the intent of the rule (root-only vs. full-tree) is ambiguous from context.

Low

General

Assert all matching calls match expected type

The assertCallType helper only finds the first matching RexCall by operator name,
but multiple calls with the same operator name may exist in the plan (e.g., two
DATE(...) calls). If the first found call does not match the expected
type/precision, the assertion will fail misleadingly. Consider collecting all
matching calls and asserting that at least one (or all) match the expected type.

api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [196-224]

 private static void assertCallType(
     RelNode plan, String operatorName, SqlTypeName expectedType, int expectedPrecision) {
-  AtomicReference<RexCall> ref = new AtomicReference<>();
+  List<RexCall> found = new ArrayList<>();
   plan.accept(
       new RelHomogeneousShuttle() {
         @Override
         public RelNode visit(RelNode other) {
           RelNode visited = super.visit(other);
           visited.accept(
               new RexShuttle() {
                 @Override
                 public RexNode visitCall(RexCall call) {
-                  if (ref.get() == null
-                      && call.getOperator().getName().equalsIgnoreCase(operatorName)) {
-                    ref.set(call);
+                  if (call.getOperator().getName().equalsIgnoreCase(operatorName)) {
+                    found.add(call);
                   }
                   return super.visitCall(call);
                 }
               });
           return visited;
         }
       });
+  assertFalse("No RexCall found for: " + operatorName, found.isEmpty());
+  for (RexCall call : found) {
+    assertEquals(operatorName + " type", expectedType, call.getType().getSqlTypeName());
+    if (expectedPrecision >= 0) {
+      assertEquals(operatorName + " precision", expectedPrecision, call.getType().getPrecision());
+    }
+  }
+}

Suggestion importance[1-10]: 4

__

Why: The suggestion improves test robustness by checking all matching RexCall nodes rather than just the first one, which could catch cases where some calls have incorrect types. However, this is a test-quality improvement with limited impact on correctness of the production code.

Low

Suggestions up to commit cc90d7f

Category	Suggestion	Impact
Possible issue	Prevent recursive application of output cast rule The rule visits every `RelNode` in the tree but only wraps the root — it should only apply to the top-level node, not recursively to all nodes. As written, `visit(RelNode` `other)` is called for every node in the plan tree (because `RelHomogeneousShuttle` recurses via `super.visit`), so inner nodes that happen to have datetime fields will also get a cast projection injected, corrupting the plan. The rule should not call `super.visit(other)` to recurse into children, or it should be applied only to the root node externally. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53] @Override public RelNode visit(RelNode other) { + // Do NOT recurse into children; this rule is intended only for the root output node. List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } - ... + + RexBuilder rexBuilder = other.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } return LogicalProject.create(other, List.of(), projects, names); } Suggestion importance[1-10]: 7 __ Why: The `DatetimeOutputCastRule` extends `RelHomogeneousShuttle` which recurses into children by default via `super.visit()`. Since the rule doesn't call `super.visit(other)`, it actually doesn't recurse — but the suggestion raises a valid concern about intent. However, looking at the code, `visit(RelNode other)` does NOT call `super.visit(other)`, so it won't recurse into children. The suggestion misreads the code but the underlying concern about applying casts to inner nodes is worth noting if the implementation were to change.	Medium
Possible issue	Avoid shared mutable shuttle singleton for thread safety `RelHomogeneousShuttle` (and `RelShuttle` implementations in general) are not thread-safe because they maintain mutable traversal state. Sharing a single `INSTANCE` across concurrent query planning calls can lead to race conditions. Each call to `postAnalysisRules()` should return a new instance instead of a shared singleton. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java [29] -static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule(); +// Remove INSTANCE singleton; instantiate per use in DatetimeExtension.postAnalysisRules() +// In DatetimeExtension: +// return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule()); Suggestion importance[1-10]: 6 __ Why: The `INSTANCE` singleton pattern for `RelHomogeneousShuttle` subclasses can be problematic in concurrent environments since shuttles may maintain traversal state. The suggestion to instantiate per use is valid for thread safety, though the actual risk depends on whether the shuttle maintains mutable state during traversal.	Low
General	Guard against null plan after shuttle application `plan.accept(shuttle)` returns a `RelNode` but `RelShuttle.accept` may return the same node or a new one. If a shuttle returns `null` (e.g., due to an unhandled node type), the next iteration will throw a `NullPointerException`. Add a null-check or assert after each shuttle application to fail fast with a meaningful error. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] RelNode plan = strategy.plan(query); for (var shuttle : context.getLangSpec().postAnalysisRules()) { - plan = plan.accept(shuttle); + RelNode result = plan.accept(shuttle); + if (result == null) { + throw new IllegalStateException( + "Post-analysis rule " + shuttle.getClass().getSimpleName() + " returned null plan"); + } + plan = result; } return plan; Suggestion importance[1-10]: 3 __ Why: While adding a null-check is a defensive practice, `RelShuttle` implementations in Calcite are not expected to return `null` — they always return a `RelNode`. This is a low-impact defensive check that adds verbosity without addressing a realistic failure scenario.	Low

Suggestions up to commit cc90d7f

Category	Suggestion	Impact
Possible issue	Return new rule instances to avoid shared state Both `DatetimeUdtNormalizeRule` and `DatetimeOutputCastRule` extend `RelHomogeneousShuttle`, which is stateful (it tracks visited nodes). Returning shared `INSTANCE` singletons means concurrent or sequential queries will share and corrupt that state. Return new instances on each call instead. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java [23-26] @Override public List<RelShuttle> postAnalysisRules() { - return List.of(DatetimeUdtNormalizeRule.INSTANCE, DatetimeOutputCastRule.INSTANCE); + return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule()); } Suggestion importance[1-10]: 7 __ Why: `RelHomogeneousShuttle` maintains a visited-node set as internal state, making shared singletons unsafe for concurrent or even sequential use. Returning new instances per call is a correct and important fix, and the `improved_code` accurately reflects the change needed.	Medium
	Prevent recursive application of output cast rule The rule visits every `RelNode` in the tree but only wraps the root — it should only apply to the top-level node. As written, it will also wrap intermediate nodes (e.g., inner `LogicalProject`s), causing redundant or incorrect double-casting of datetime fields. The rule should first recurse into children and then only apply the cast projection to the root, or it should be applied only once on the root outside the shuttle. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53] @Override public RelNode visit(RelNode other) { + // Do NOT recurse into children; this rule is intended for the root node only. List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } - ... + + RexBuilder rexBuilder = other.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } return LogicalProject.create(other, List.of(), projects, names); } Suggestion importance[1-10]: 5 __ Why: The concern about recursive application is valid in principle, but looking at the test cases (e.g., `testAllStandardDatetimeTypesCastToVarchar`), the expected plan shows the cast only on the outer `LogicalProject`, suggesting the current behavior may be intentional or the tests validate the actual behavior. The `improved_code` removes the call to `super.visit()` which would break child traversal needed by `DatetimeUdtNormalizeRule`. The suggestion has merit but the improved code doesn't accurately reflect a safe fix.	Low
	Avoid shared mutable shuttle singleton for thread safety `RelHomogeneousShuttle` (and `RelShuttle` in general) is not thread-safe because it maintains internal state (visited set). Sharing a single `INSTANCE` across concurrent queries will cause race conditions. Each call to `postAnalysisRules()` should return a new instance instead of a shared singleton. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java [29] -static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule(); +// Remove INSTANCE singleton; instantiate per use in DatetimeExtension.postAnalysisRules() +// In DatetimeExtension: +// return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule()); Suggestion importance[1-10]: 3 __ Why: The thread-safety concern about `RelHomogeneousShuttle` is valid, but the `improved_code` is just a comment rather than actual code, making it an incomplete suggestion. The actual fix should be in `DatetimeExtension`, which is covered by suggestion 3.	Low

Suggestions up to commit 01fba06

Category	Suggestion	Impact
Possible issue	Recursively visit children before applying cast rule The rule does not recursively visit child nodes before applying the cast. If a datetime field appears in a nested node (not the root), it will be missed. Call `super.visit(other)` first to ensure children are visited before processing the current node. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-36] @Override public RelNode visit(RelNode other) { - List<RelDataTypeField> fields = other.getRowType().getFieldList(); + RelNode visited = super.visit(other); + List<RelDataTypeField> fields = visited.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { - return other; + return visited; } + RexBuilder rexBuilder = visited.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(visited, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } + return LogicalProject.create(visited, List.of(), projects, names); +} Suggestion importance[1-10]: 6 __ Why: The `DatetimeOutputCastRule` doesn't call `super.visit(other)` before processing, meaning it only applies to the root node and misses datetime fields in nested nodes. The `DatetimeUdtNormalizeRule` correctly calls `super.visit(other)` first, so this inconsistency could be a real issue depending on the intended behavior. However, since this rule is described as wrapping "the root output", it may be intentional to only apply at the top level.	Low
General	Close ResultSet to prevent resource leaks The `ResultSet` is not closed after use, which can cause resource leaks. Wrap it in a try-with-resources block to ensure it is always closed. api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [167-174] -ResultSet resultSet = statement.executeQuery(); -verify(resultSet) - .expectSchema( - col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) - .expectData( - row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), - row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +try (ResultSet resultSet = statement.executeQuery()) { + verify(resultSet) + .expectSchema( + col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) + .expectData( + row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), + row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +} Suggestion importance[1-10]: 4 __ Why: The `ResultSet` is not closed after use, which can cause resource leaks. Wrapping it in a try-with-resources block is a valid improvement, though the impact is lower in test code since resources are typically cleaned up when the test JVM exits.	Low
General	Avoid shared mutable shuttle instances across queries `RelShuttle.accept` returns a `RelNode`, but `RelHomogeneousShuttle` implementations may return the same instance if no changes are made. However, the shuttles used here (`DatetimeUdtNormalizeRule`, `DatetimeOutputCastRule`) are stateful singletons shared across calls. If these shuttles accumulate any internal state between invocations, concurrent or sequential queries could interfere. Consider instantiating new shuttle instances per query rather than using shared `INSTANCE` singletons. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] +RelNode plan = strategy.plan(query); +for (var shuttle : context.getLangSpec().postAnalysisRules()) { + plan = plan.accept(shuttle); +} +return plan; - Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about singleton shuttle instances, but `RelHomogeneousShuttle` is typically stateless and the `improved_code` is identical to the `existing_code`, making this more of a verification suggestion than an actionable fix.	Low

Suggestions up to commit 70aabfe

Category	Suggestion	Impact
Possible issue	Missing recursive traversal of child nodes The `visit` method does not call `super.visit(other)` before processing the node, meaning child nodes are never recursively visited. This causes the rule to only apply to the root node and miss datetime fields in nested `RelNode`s. Call `super.visit(other)` first and operate on the returned node, consistent with how `DatetimeUdtNormalizeRule` handles this. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [28-32] public RelNode visit(RelNode other) { - List<RelDataTypeField> fields = other.getRowType().getFieldList(); + RelNode visited = super.visit(other); + List<RelDataTypeField> fields = visited.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { - return other; + return visited; } Suggestion importance[1-10]: 7 __ Why: The `DatetimeOutputCastRule.visit` method doesn't call `super.visit(other)` unlike `DatetimeUdtNormalizeRule`, meaning child nodes are never recursively visited. However, the rule is designed to wrap the root output with CAST for wire-format compatibility, so it may be intentionally applied only at the top level. The inconsistency with `DatetimeUdtNormalizeRule` is worth noting, but the impact depends on the intended behavior.	Medium
General	ResultSet resource not closed after use The `ResultSet` is never closed after use, which can leak JDBC resources. Wrap `resultSet` in a try-with-resources block or close it in a `finally` block to ensure proper cleanup. api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [167-174] -ResultSet resultSet = statement.executeQuery(); -verify(resultSet) - .expectSchema( - col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) - .expectData( - row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), - row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +try (ResultSet resultSet = statement.executeQuery()) { + verify(resultSet) + .expectSchema( + col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) + .expectData( + row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), + row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +} Suggestion importance[1-10]: 5 __ Why: The `ResultSet` is not closed after use, which can leak JDBC resources. The suggested fix of wrapping in try-with-resources is correct and the `improved_code` accurately reflects the change, though this is a test file so the impact is limited.	Low
General	Shared shuttle singletons risk concurrency issues `RelShuttle.accept` returns a `RelNode`, but `RelHomogeneousShuttle` instances are stateful — reusing the same `INSTANCE` singleton across multiple concurrent queries could cause race conditions if the shuttle holds any mutable state. Consider instantiating a new shuttle per invocation rather than using shared singletons (`DatetimeUdtNormalizeRule.INSTANCE`, `DatetimeOutputCastRule.INSTANCE`), or ensure the shuttles are truly stateless. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] +RelNode plan = strategy.plan(query); +for (var shuttle : context.getLangSpec().postAnalysisRules()) { + plan = plan.accept(shuttle); +} +return plan; - Suggestion importance[1-10]: 4 __ Why: The concern about singleton `INSTANCE` shuttles being stateful is valid in principle, but the `improved_code` is identical to `existing_code`, making this more of a verification suggestion. The actual fix would need to be in `DatetimeExtension.postAnalysisRules()` to return new instances, not in `UnifiedQueryPlanner`.	Low

github-actions · 2026-05-06T18:33:48Z

Persistent review updated to latest commit 01fba06

dai-chen · 2026-05-06T18:42:22Z

All test related CI is failling on Gradle version issue:

* What went wrong:
A problem occurred evaluating root project 'opensearch-sql'.
> Failed to apply plugin class 'org.opensearch.gradle.info.GlobalBuildInfoPlugin'.
   > Gradle 9.4.1+ is required

github-actions · 2026-05-07T01:55:17Z

Persistent review updated to latest commit cc90d7f

dai-chen · 2026-05-07T02:07:43Z

All test related CI is failling on Gradle version issue:

* What went wrong:
A problem occurred evaluating root project 'opensearch-sql'.
> Failed to apply plugin class 'org.opensearch.gradle.info.GlobalBuildInfoPlugin'.
   > Gradle 9.4.1+ is required

Will rebase once #5414 merged.

github-actions · 2026-05-07T02:55:27Z

Persistent review updated to latest commit af2b288

Add postAnalysisRules (List<RelShuttle>) to LanguageSpec.LanguageExtension and register DatetimeExtension in UnifiedPplSpec with two rules: 1. DatetimeUdtNormalizeRule rewrites datetime UDT return types (EXPR_DATE/TIME/TIMESTAMP) on RexCall nodes to standard Calcite DATE/TIME(9)/TIMESTAMP(9) types via call.clone(). Precision is derived from the type system (OpenSearchTypeSystem.getMaxPrecision). 2. DatetimeOutputCastRule adds a final LogicalProject that casts standard datetime output columns to VARCHAR, aligning with PPL's wire-format contract (ISO string representation). Both rules run as postAnalysisRules after the planning strategy produces the RelNode, applied uniformly to both SQL and PPL paths. Also bumps OpenSearchTypeSystem max datetime precision from 3 to 9 (nanosecond) for TIME and TIMESTAMP types. No changes to UDF definitions or implementors in core/ — the mismatch between rewritten signatures and UDF implementations is a known limitation addressed separately. Signed-off-by: Chen Dai <daichen@amazon.com>

@ignore

Bring the analytics-engine catch-up PR up to current upstream/main by resolving conflicts introduced by 4 main commits since 2026-04-30: - opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - opensearch-project#5408 (datetime type normalization) - opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion) - opensearch-project#5399 (FGAC-scoped SQL cursor continuation) Resolutions: api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. PR's RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in opensearch-project#5408 / opensearch-project#5419. core/executor/QueryService.java: composed both sides — kept HEAD's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking. Same pattern as the original PR resolution; both improvements are orthogonal. plugin/SQLPlugin.java: kept HEAD imports for ExplainResponse and QueryType (referenced by createSqlAnalyticsRouter, which only exists on the feature branch). plugin/transport/TransportPPLQueryAction.java: kept HEAD's queryPlanExecutor parameter alongside main's extensionsHolder parameter, since both are referenced in the constructor body. legacy/plugin/RestSqlAction.java: took HEAD. The 3-way merge produced a duplicated handleException/getRawErrorCode block; HEAD already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so HEAD is the correct superset. Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava and unit tests pass; spotlessCheck passes. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

…ation Brings the catch-up branch up to current upstream/main (4 commits since this PR was opened) and current feature/mustang-ppl-integration (9 commits since this PR was opened), so the PR is mergeable into feature/mustang-ppl-integration without conflicts. Squashed (rather than two real merge commits) for the same DCO reason the original commit was squashed: upstream commits authored by many contributors with inconsistent or missing Signed-off-by trailers would otherwise be brought into this PR's history. Newer main commits absorbed (4): - opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - opensearch-project#5408 (datetime type normalization) - opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion) - opensearch-project#5399 (FGAC-scoped SQL cursor continuation) Newer feature commits absorbed (9): - opensearch-project#5403 (analytics-engine optional dependency — major rewiring) - opensearch-project#5407 (Carry CalciteEvalCommandIT through helper-managed index path) - opensearch-project#5413 (Default plugins.calcite.enabled=true on unified path) - opensearch-project#5415, opensearch-project#5416, opensearch-project#5417, opensearch-project#5409, opensearch-project#5400, opensearch-project#5406 (smaller carryovers + bumps) Conflict resolutions (10 from main side, 3 from feature side): api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. PR's RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in opensearch-project#5408 / opensearch-project#5419. core/executor/QueryService.java: composed both sides — kept HEAD's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking. Same pattern as the original PR resolution; both improvements are orthogonal. legacy/plugin/RestSqlAction.java: took HEAD. The 3-way merge produced a duplicated handleException/getRawErrorCode block; HEAD already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so HEAD is the correct superset. integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 TestEventReporterAsListener cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs get a parquet-backed index. Added back PR HEAD's test_eval_agent setup (needed by the dotted-path eval tests for opensearch-project#5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java: took feature. PR opensearch-project#5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter. Feature's design supersedes our prior wiring. plugin/.../SQLPlugin.java: took feature. The same opensearch-project#5403 simplification removed loadExtensions/EngineExtensionsHolder/executionEngineExtensions plumbing (no longer needed once analytics-engine is optionally bound). Feature retains the createSqlAnalyticsRouter method this PR introduced. plugin/.../config/EngineExtensionsHolder.java: deleted. Unreferenced after taking feature's SQLPlugin/TransportPPLQueryAction; not present on feature branch. Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava + :integ-test compileTestJava all pass; unit tests pass; spotlessCheck clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

Single squashed commit on top of feature/mustang-ppl-integration that absorbs upstream/main's commits not yet on the feature branch. Replaces the prior catch-up squash (opensearch-project#5396 base + the original af831d3 rebase commit) so this PR is a fast-forward into feature/mustang-ppl-integration. Squashed (rather than a merge commit) because upstream main commits were authored by many contributors with inconsistent or missing Signed-off-by trailers; DCO would otherwise reject those commits. Main commits absorbed (54 since divergence; 4 since the original catch-up squash was made on 2026-04-30): - opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - opensearch-project#5408 (datetime type normalization) - opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion) - opensearch-project#5399 (FGAC-scoped SQL cursor continuation) - opensearch-project#5394 (SQL Vector Search), opensearch-project#5361 (OpenSearch 3.7), opensearch-project#5360 (unified SQL language spec), opensearch-project#5240 (PPL Union), and 46 others. Conflict resolutions: api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in opensearch-project#5408 / opensearch-project#5419. core/executor/QueryService.java: composed both sides — kept feature's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking; both improvements are orthogonal. legacy/plugin/RestSqlAction.java: took feature. The 3-way merge produced a duplicated handleException/getRawErrorCode block; feature already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so feature is the correct superset. CLAUDE.md, docs/user/ppl/functions/condition.md: took main. explain_streamstats_global{,_null_bucket}.yaml: took main (post-opensearch-project#5359 shape). core/CalciteRelNodeVisitor + utils/PlanUtils: took main (collation utility hoisted from CalciteRelNodeVisitor.backtrackForCollation into PlanUtils.findInputCollation). integ-test/CalciteNoPushdownIT.java: added CalciteMixedFieldTypeIT. ppl/antlr/OpenSearchPPLParser.g4: added unionCommand. ppl/calcite/CalcitePPLStreamstatsTest.java: added testMultipleStreamstatsWithWindow. integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs get a parquet-backed index. Added the test_eval_agent setup (needed by the dotted-path eval tests for opensearch-project#5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java, plugin/.../SQLPlugin.java: took feature. PR opensearch-project#5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter, and removed the loadExtensions / EngineExtensionsHolder / executionEngineExtensions plumbing. Feature retains the createSqlAnalyticsRouter method this catch-up introduced. plugin/.../config/EngineExtensionsHolder.java: deleted (unreferenced post-opensearch-project#5403; not present on feature). Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava + :integ-test compileTestJava all pass; unit tests pass; spotlessCheck clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

…5397) Single squashed commit on top of feature/mustang-ppl-integration that absorbs upstream/main's commits not yet on the feature branch. Replaces the prior catch-up squash (#5396 base + the original af831d3 rebase commit) so this PR is a fast-forward into feature/mustang-ppl-integration. Squashed (rather than a merge commit) because upstream main commits were authored by many contributors with inconsistent or missing Signed-off-by trailers; DCO would otherwise reject those commits. Main commits absorbed (54 since divergence; 4 since the original catch-up squash was made on 2026-04-30): - #5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - #5408 (datetime type normalization) - #5414 (Gradle wrapper bump + @ignore exclusion) - #5399 (FGAC-scoped SQL cursor continuation) - #5394 (SQL Vector Search), #5361 (OpenSearch 3.7), #5360 (unified SQL language spec), #5240 (PPL Union), and 46 others. Conflict resolutions: api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in #5408 / #5419. core/executor/QueryService.java: composed both sides — kept feature's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking; both improvements are orthogonal. legacy/plugin/RestSqlAction.java: took feature. The 3-way merge produced a duplicated handleException/getRawErrorCode block; feature already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so feature is the correct superset. CLAUDE.md, docs/user/ppl/functions/condition.md: took main. explain_streamstats_global{,_null_bucket}.yaml: took main (post-#5359 shape). core/CalciteRelNodeVisitor + utils/PlanUtils: took main (collation utility hoisted from CalciteRelNodeVisitor.backtrackForCollation into PlanUtils.findInputCollation). integ-test/CalciteNoPushdownIT.java: added CalciteMixedFieldTypeIT. ppl/antlr/OpenSearchPPLParser.g4: added unionCommand. ppl/calcite/CalcitePPLStreamstatsTest.java: added testMultipleStreamstatsWithWindow. integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from #5407) so analytics-engine compatibility runs get a parquet-backed index. Added the test_eval_agent setup (needed by the dotted-path eval tests for #5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java, plugin/.../SQLPlugin.java: took feature. PR #5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter, and removed the loadExtensions / EngineExtensionsHolder / executionEngineExtensions plumbing. Feature retains the createSqlAnalyticsRouter method this catch-up introduced. plugin/.../config/EngineExtensionsHolder.java: deleted (unreferenced post-#5403; not present on feature). Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava + :integ-test compileTestJava all pass; unit tests pass; spotlessCheck clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

Brings the branch up to current upstream/main (54 commits since the last catch-up at opensearch-project#5397, divergence point 513e1b2). Preserves both parents so commit lineage from main is retained on the feature branch. ### Main commits absorbed (54 since divergence) Highlights: - opensearch-project#5419 Register LENGTH/REGEXP_REPLACE/DATE_TRUNC in unified function spec - opensearch-project#5408 Normalize datetime types for unified query API - opensearch-project#5414 Gradle wrapper 9.4.1 bump + exclude @ignore classes - opensearch-project#5399 [BugFix] Scope SQL cursor continuation to original query indices under FGAC - opensearch-project#5394 [Feature] Support SQL Vector Search - opensearch-project#5361 Version bump to OpenSearch 3.7 (Jackson 2 → 3 parser API) - opensearch-project#5360 Define unified SQL language spec with composable extensions - opensearch-project#5240 [FEATURE] Union command in PPL - opensearch-project#5266 Initial implementation of report-builder interface - opensearch-project#5278 isnotnull condition support - opensearch-project#3922, opensearch-project#4659, opensearch-project#4800, opensearch-project#5099, opensearch-project#5169, opensearch-project#5172, opensearch-project#5175, opensearch-project#5185, opensearch-project#5347, opensearch-project#5370 and other bugfixes - 34 others (bugfixes, doc updates, infra) ### Conflict resolutions Seven content conflicts resolved during the merge. Resolution kept the feature branch's analytics-engine-path semantics where main's changes would have regressed them. - api/.../UnifiedQueryContext.java Blank-line-only conflict in Builder.build(); took main's tighter formatting (no extra blank). - core/.../executor/QueryService.java Main reformatted executeWithCalcite without the CalciteClassLoaderHelper.withCalciteClassLoader() wrapper. Kept the feature-branch wrapping — required for analytics-engine classloader isolation — and the matching import. Same shape applies to explainWithCalcite which git auto-merged correctly. - integ-test/build.gradle Both sides added @Ignore-class exclusions to work around the Gradle 9.4.1 TestEventReporterAsListener ClassCastException. Took the feature-branch's detailed root-cause comment, kept ASCII ordering of the JSONRequestIT/JoinIT and SQLFunctionsIT/ShowIT/SourceFieldIT entries. - integ-test/.../CalciteEvalCommandIT.java Feature branch wraps test-data PUTs in if (!TestUtils.isIndexExist(...)) for idempotency on the helper-managed-index analytics-engine compatibility run; main has the unguarded PUTs. Kept feature's idempotency guards for both test_eval and test_eval_agent. - legacy/.../RestSqlAction.java Feature branch added delegateToV2Engine(...) (extracted from the analytics-engine routing path) and ALSO has handleException / getRestStatus / getRawErrorCode; main added the latter three in the same position. Kept feature's delegateToV2Engine, kept one copy of the helpers, removed the duplicate set that git produced. - plugin/.../SQLPlugin.java Both sides expanded the executor import block. Kept the union: ExecutionEngine + ExecutionEngine.ExplainResponse + QueryType (all three are referenced in this file). - plugin/.../transport/TransportPPLQueryAction.java Main now passes engine extensions to OpenSearchPluginModule via extensionsHolder.engines(). Feature creates a local pluginSettings for the unified-query handler. Combined both — kept the extensions-aware module construction *and* the pluginSettings / pluginSettingsRef wiring. EngineExtensionsHolder is a new file from main (opensearch-project#5298) preserved as-is. ### Verification - ./gradlew :api:compileJava :core:compileJava :legacy:compileJava :opensearch-sql-plugin:compileJava :integ-test:compileTestJava passes locally Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

Single squashed delivery of the long-running feature/mustang-ppl-integration branch into main, consolidating 22 feature-branch PRs plus the conflict-resolved merge of current main. Squashed because the feature branch's history includes commits with missing or mismatched Signed-off-by trailers that block DCO at this scope — the equivalent issue documented for the catch-up squashes (opensearch-project#5397). The feature branch f006e29 is retained for individual-commit lineage. ### What this delivers Analytics-engine PPL integration — a new execution path that routes Parquet-backed (non-Lucene) indices through an analytics engine while keeping Lucene-backed indices on the existing v2 / Calcite paths. Headline pieces: - Query routing (opensearch-project#5267) — PPL queries against Parquet-backed indices hand off to the analytics-engine execution path; Lucene-backed indices continue through the legacy path - Explain support (opensearch-project#5275) — EXPLAIN covers the analytics-engine path - Profiling + UnifiedQueryParser (opensearch-project#5285) — migrates PPL parsing to the unified parser and wires profiling metrics through the analytics path - extendedPlugins wiring (opensearch-project#5302) — analytics-engine attaches as an OpenSearch extension via SPI - SQL REST endpoint integration (opensearch-project#5317) — same analytics-route fork applied to the SQL transport, plus delegateToV2Engine extraction in RestSqlAction - Async QueryPlanExecutor (opensearch-project#5396) — async execution for analytics-engine plans + version bump to OpenSearch 3.7 - Optional dependency (opensearch-project#5403) — analytics-engine becomes an optional runtime dep so the SQL bundle is shippable without it - Index-setting-based routing (opensearch-project#5429) — replaces the earlier table-name-prefix heuristic with an authoritative index-setting check Supporting infrastructure: - Gradle wrapper bump to 9.4.1 (opensearch-project#5406) - Jar-hell exclusions for arrow-flight-rpc / httpcore5-h2 / httpcore5-reactive / httpclient5 (opensearch-project#5400, opensearch-project#5409) - IT plumbing: CalciteEvalCommandIT / CalciteFieldFormatCommandIT carried through the helper-managed index path (opensearch-project#5407, opensearch-project#5417); CalciteReplaceCommandIT column-order-agnostic (opensearch-project#5415); @ignore'd Calcite ITs dropped from CalciteNoPushdownIT (opensearch-project#5416) - plugins.calcite.enabled=true defaulted on the unified query path (opensearch-project#5413) - PPL_REX_MAX_MATCH_LIMIT bridged into UnifiedQueryContext (opensearch-project#5418) - Calcite tolerance fixes: array() default type (opensearch-project#5421), containsNestedAggregator flat-leaf schemas (opensearch-project#5423) - Sandbox deps switched to analytics-api JDK 21 surface (opensearch-project#5426) ### Feature-branch commits squashed (22) opensearch-project#5429, opensearch-project#5426, opensearch-project#5423, opensearch-project#5421, opensearch-project#5418, opensearch-project#5403, opensearch-project#5417, opensearch-project#5415, opensearch-project#5416, opensearch-project#5413, opensearch-project#5407, opensearch-project#5409, opensearch-project#5406, opensearch-project#5400, opensearch-project#5396, opensearch-project#5317, opensearch-project#5302, opensearch-project#5285, opensearch-project#5275, opensearch-project#5267, opensearch-project#5397, opensearch-project#5286 ### Main commits absorbed via the merge (54) Brings the branch up to current upstream/main (54 commits since the last catch-up at opensearch-project#5397, divergence point 513e1b2). Highlights: opensearch-project#5419, opensearch-project#5408, opensearch-project#5414, opensearch-project#5399, opensearch-project#5394, opensearch-project#5361, opensearch-project#5360, opensearch-project#5240, opensearch-project#5266, opensearch-project#5278, plus 44 others (bugfixes, doc updates, infra). ### Conflict resolutions (7) Resolved during the merge of main into the feature branch. Resolution kept the feature branch's analytics-engine-path semantics where main's changes would have regressed them. - api/.../UnifiedQueryContext.java Blank-line-only conflict; took main's tighter formatting. - core/.../executor/QueryService.java Kept feature's CalciteClassLoaderHelper.withCalciteClassLoader(...) wrapping (required for analytics-engine classloader isolation) and the matching import. - integ-test/build.gradle Kept feature's detailed root-cause comment on the Gradle 9.4.1 TestEventReporterAsListener workaround; kept ASCII ordering of JSONRequestIT / JoinIT and SQLFunctionsIT / ShowIT / SourceFieldIT entries. - integ-test/.../CalciteEvalCommandIT.java Kept feature's if (!TestUtils.isIndexExist(...)) idempotency guards on test_eval and test_eval_agent setup (needed for the helper-managed index analytics-engine compatibility run). - legacy/.../RestSqlAction.java Kept feature's delegateToV2Engine(...) (extracted from the analytics-engine routing path). Both sides added handleException / getRestStatus / getRawErrorCode; removed the duplicate set git produced. - plugin/.../SQLPlugin.java Took the union of imports: ExecutionEngine + ExecutionEngine.ExplainResponse + QueryType. - plugin/.../transport/TransportPPLQueryAction.java Combined main's OpenSearchPluginModule(extensionsHolder.engines()) and feature's local pluginSettings / pluginSettingsRef wiring. EngineExtensionsHolder.java is a new file from main (opensearch-project#5298) preserved as-is. ### Compatibility / opt-in The analytics-engine path is gated by the extendedPlugins extension being installed (opensearch-project#5403 makes the dep optional). Clusters without analytics-engine installed see no behavior change. Clusters with analytics-engine installed route only Parquet-backed indices through the new path (opensearch-project#5429 — by index setting). ### Verification - ./gradlew :api:compileJava :core:compileJava :legacy:compileJava :opensearch-sql-plugin:compileJava :integ-test:compileTestJava passes locally Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

Single squashed delivery of the long-running feature/mustang-ppl-integration branch into main, consolidating 22 feature-branch PRs plus the conflict-resolved merge of current main. Squashed because the feature branch's history includes commits with missing or mismatched Signed-off-by trailers that block DCO at this scope — the equivalent issue documented for the catch-up squashes (opensearch-project#5397). The feature branch f006e29 is retained for individual-commit lineage. ### What this delivers Analytics-engine PPL integration — a new execution path that routes Parquet-backed (non-Lucene) indices through an analytics engine while keeping Lucene-backed indices on the existing v2 / Calcite paths. Headline pieces: - Query routing (opensearch-project#5267) — PPL queries against Parquet-backed indices hand off to the analytics-engine execution path; Lucene-backed indices continue through the legacy path - Explain support (opensearch-project#5275) — EXPLAIN covers the analytics-engine path - Profiling + UnifiedQueryParser (opensearch-project#5285) — migrates PPL parsing to the unified parser and wires profiling metrics through the analytics path - extendedPlugins wiring (opensearch-project#5302) — analytics-engine attaches as an OpenSearch extension via SPI - SQL REST endpoint integration (opensearch-project#5317) — same analytics-route fork applied to the SQL transport, plus delegateToV2Engine extraction in RestSqlAction - Async QueryPlanExecutor (opensearch-project#5396) — async execution for analytics-engine plans + version bump to OpenSearch 3.7 - Optional dependency (opensearch-project#5403) — analytics-engine becomes an optional runtime dep so the SQL bundle is shippable without it - Index-setting-based routing (opensearch-project#5429, opensearch-project#5432) — replaces the earlier table-name-prefix heuristic with an authoritative index-setting check Supporting infrastructure: - Gradle wrapper bump to 9.4.1 (opensearch-project#5406) - Jar-hell exclusions for arrow-flight-rpc / httpcore5-h2 / httpcore5-reactive / httpclient5 (opensearch-project#5400, opensearch-project#5409) - IT plumbing: CalciteEvalCommandIT / CalciteFieldFormatCommandIT carried through the helper-managed index path (opensearch-project#5407, opensearch-project#5417); CalciteReplaceCommandIT column-order-agnostic (opensearch-project#5415); @ignore'd Calcite ITs dropped from CalciteNoPushdownIT (opensearch-project#5416) - plugins.calcite.enabled=true defaulted on the unified query path (opensearch-project#5413) - PPL_REX_MAX_MATCH_LIMIT bridged into UnifiedQueryContext (opensearch-project#5418) - Calcite tolerance fixes: array() default type (opensearch-project#5421), containsNestedAggregator flat-leaf schemas (opensearch-project#5423) - Sandbox deps switched to analytics-api JDK 21 surface (opensearch-project#5426) ### Feature-branch commits squashed (22) opensearch-project#5432, opensearch-project#5429, opensearch-project#5426, opensearch-project#5423, opensearch-project#5421, opensearch-project#5418, opensearch-project#5403, opensearch-project#5417, opensearch-project#5415, opensearch-project#5416, opensearch-project#5413, opensearch-project#5407, opensearch-project#5409, opensearch-project#5406, opensearch-project#5400, opensearch-project#5396, opensearch-project#5317, opensearch-project#5302, opensearch-project#5285, opensearch-project#5275, opensearch-project#5267, opensearch-project#5397, opensearch-project#5286 ### Main commits absorbed via the merge (54) Brings the branch up to current upstream/main (54 commits since the last catch-up at opensearch-project#5397, divergence point 513e1b2). Highlights: opensearch-project#5419, opensearch-project#5408, opensearch-project#5414, opensearch-project#5399, opensearch-project#5394, opensearch-project#5361, opensearch-project#5360, opensearch-project#5240, opensearch-project#5266, opensearch-project#5278, plus 44 others (bugfixes, doc updates, infra). ### Conflict resolutions (7) Resolved during the merge of main into the feature branch. Resolution kept the feature branch's analytics-engine-path semantics where main's changes would have regressed them. - api/.../UnifiedQueryContext.java Blank-line-only conflict; took main's tighter formatting. - core/.../executor/QueryService.java Kept feature's CalciteClassLoaderHelper.withCalciteClassLoader(...) wrapping (required for analytics-engine classloader isolation) and the matching import. - integ-test/build.gradle Kept feature's detailed root-cause comment on the Gradle 9.4.1 TestEventReporterAsListener workaround; kept ASCII ordering of JSONRequestIT / JoinIT and SQLFunctionsIT / ShowIT / SourceFieldIT entries. - integ-test/.../CalciteEvalCommandIT.java Kept feature's if (!TestUtils.isIndexExist(...)) idempotency guards on test_eval and test_eval_agent setup (needed for the helper-managed index analytics-engine compatibility run). - legacy/.../RestSqlAction.java Kept feature's delegateToV2Engine(...) (extracted from the analytics-engine routing path). Both sides added handleException / getRestStatus / getRawErrorCode; removed the duplicate set git produced. - plugin/.../SQLPlugin.java Took the union of imports: ExecutionEngine + ExecutionEngine.ExplainResponse + QueryType. - plugin/.../transport/TransportPPLQueryAction.java Combined main's OpenSearchPluginModule(extensionsHolder.engines()) and feature's local pluginSettings / pluginSettingsRef wiring. EngineExtensionsHolder.java is a new file from main (opensearch-project#5298) preserved as-is. ### Compatibility / opt-in The analytics-engine path is gated by the extendedPlugins extension being installed (opensearch-project#5403 makes the dep optional). Clusters without analytics-engine installed see no behavior change. Clusters with analytics-engine installed route only Parquet-backed indices through the new path (opensearch-project#5429 — by index setting). ### Verification - ./gradlew :api:compileJava :core:compileJava :legacy:compileJava :opensearch-sql-plugin:compileJava :integ-test:compileTestJava passes locally Signed-off-by: Kai Huang <ahkcs@amazon.com> Co-authored-by: bowenlan-amzn <bowenlan23@gmail.com>

@ignore

* Land analytics-engine PPL integration into main Single squashed delivery of the long-running feature/mustang-ppl-integration branch into main, consolidating 22 feature-branch PRs plus the conflict-resolved merge of current main. Squashed because the feature branch's history includes commits with missing or mismatched Signed-off-by trailers that block DCO at this scope — the equivalent issue documented for the catch-up squashes (#5397). The feature branch f006e29 is retained for individual-commit lineage. ### What this delivers Analytics-engine PPL integration — a new execution path that routes Parquet-backed (non-Lucene) indices through an analytics engine while keeping Lucene-backed indices on the existing v2 / Calcite paths. Headline pieces: - Query routing (#5267) — PPL queries against Parquet-backed indices hand off to the analytics-engine execution path; Lucene-backed indices continue through the legacy path - Explain support (#5275) — EXPLAIN covers the analytics-engine path - Profiling + UnifiedQueryParser (#5285) — migrates PPL parsing to the unified parser and wires profiling metrics through the analytics path - extendedPlugins wiring (#5302) — analytics-engine attaches as an OpenSearch extension via SPI - SQL REST endpoint integration (#5317) — same analytics-route fork applied to the SQL transport, plus delegateToV2Engine extraction in RestSqlAction - Async QueryPlanExecutor (#5396) — async execution for analytics-engine plans + version bump to OpenSearch 3.7 - Optional dependency (#5403) — analytics-engine becomes an optional runtime dep so the SQL bundle is shippable without it - Index-setting-based routing (#5429, #5432) — replaces the earlier table-name-prefix heuristic with an authoritative index-setting check Supporting infrastructure: - Gradle wrapper bump to 9.4.1 (#5406) - Jar-hell exclusions for arrow-flight-rpc / httpcore5-h2 / httpcore5-reactive / httpclient5 (#5400, #5409) - IT plumbing: CalciteEvalCommandIT / CalciteFieldFormatCommandIT carried through the helper-managed index path (#5407, #5417); CalciteReplaceCommandIT column-order-agnostic (#5415); @ignore'd Calcite ITs dropped from CalciteNoPushdownIT (#5416) - plugins.calcite.enabled=true defaulted on the unified query path (#5413) - PPL_REX_MAX_MATCH_LIMIT bridged into UnifiedQueryContext (#5418) - Calcite tolerance fixes: array() default type (#5421), containsNestedAggregator flat-leaf schemas (#5423) - Sandbox deps switched to analytics-api JDK 21 surface (#5426) ### Feature-branch commits squashed (22) #5432, #5429, #5426, #5423, #5421, #5418, #5403, #5417, #5415, #5416, #5413, #5407, #5409, #5406, #5400, #5396, #5317, #5302, #5285, #5275, #5267, #5397, #5286 ### Main commits absorbed via the merge (54) Brings the branch up to current upstream/main (54 commits since the last catch-up at #5397, divergence point 513e1b2). Highlights: #5419, #5408, #5414, #5399, #5394, #5361, #5360, #5240, #5266, #5278, plus 44 others (bugfixes, doc updates, infra). ### Conflict resolutions (7) Resolved during the merge of main into the feature branch. Resolution kept the feature branch's analytics-engine-path semantics where main's changes would have regressed them. - api/.../UnifiedQueryContext.java Blank-line-only conflict; took main's tighter formatting. - core/.../executor/QueryService.java Kept feature's CalciteClassLoaderHelper.withCalciteClassLoader(...) wrapping (required for analytics-engine classloader isolation) and the matching import. - integ-test/build.gradle Kept feature's detailed root-cause comment on the Gradle 9.4.1 TestEventReporterAsListener workaround; kept ASCII ordering of JSONRequestIT / JoinIT and SQLFunctionsIT / ShowIT / SourceFieldIT entries. - integ-test/.../CalciteEvalCommandIT.java Kept feature's if (!TestUtils.isIndexExist(...)) idempotency guards on test_eval and test_eval_agent setup (needed for the helper-managed index analytics-engine compatibility run). - legacy/.../RestSqlAction.java Kept feature's delegateToV2Engine(...) (extracted from the analytics-engine routing path). Both sides added handleException / getRestStatus / getRawErrorCode; removed the duplicate set git produced. - plugin/.../SQLPlugin.java Took the union of imports: ExecutionEngine + ExecutionEngine.ExplainResponse + QueryType. - plugin/.../transport/TransportPPLQueryAction.java Combined main's OpenSearchPluginModule(extensionsHolder.engines()) and feature's local pluginSettings / pluginSettingsRef wiring. EngineExtensionsHolder.java is a new file from main (#5298) preserved as-is. ### Compatibility / opt-in The analytics-engine path is gated by the extendedPlugins extension being installed (#5403 makes the dep optional). Clusters without analytics-engine installed see no behavior change. Clusters with analytics-engine installed route only Parquet-backed indices through the new path (#5429 — by index setting). ### Verification - ./gradlew :api:compileJava :core:compileJava :legacy:compileJava :opensearch-sql-plugin:compileJava :integ-test:compileTestJava passes locally Signed-off-by: Kai Huang <ahkcs@amazon.com> Co-authored-by: bowenlan-amzn <bowenlan23@gmail.com> * Address @penghuo: revert stray blank line in doctest/build.gradle After 'apply plugin: opensearch.testclusters', one blank line is enough — restoring the single-blank spacing to match upstream/main. Signed-off-by: Kai Huang <ahkcs@amazon.com> --------- Signed-off-by: Kai Huang <ahkcs@amazon.com> Co-authored-by: bowenlan-amzn <bowenlan23@gmail.com>

dai-chen self-assigned this May 6, 2026

dai-chen added PPL Piped processing language bugFix labels May 6, 2026

dai-chen force-pushed the feature/ppl-udt-cleanup branch from 70aabfe to 01fba06 Compare May 6, 2026 18:31

dai-chen marked this pull request as ready for review May 6, 2026 18:32

dai-chen requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners May 6, 2026 18:32

penghuo previously approved these changes May 6, 2026

View reviewed changes

mengweieric previously approved these changes May 6, 2026

View reviewed changes

dai-chen dismissed stale reviews from mengweieric and penghuo via cc90d7f May 7, 2026 01:53

dai-chen force-pushed the feature/ppl-udt-cleanup branch from 01fba06 to cc90d7f Compare May 7, 2026 01:53

dai-chen force-pushed the feature/ppl-udt-cleanup branch from cc90d7f to af2b288 Compare May 7, 2026 02:53

dai-chen force-pushed the feature/ppl-udt-cleanup branch from af2b288 to 58b0420 Compare May 7, 2026 15:33

mengweieric approved these changes May 7, 2026

View reviewed changes

penghuo approved these changes May 7, 2026

View reviewed changes

dai-chen merged commit 3ed472e into opensearch-project:main May 7, 2026
37 of 40 checks passed

ahkcs mentioned this pull request May 7, 2026

Datetime output cast format diverges between Calcite and DataFusion engines #5420

Open

dai-chen mentioned this pull request May 8, 2026

[BUG] Unified query API fails with date types #5250

Open

ahkcs mentioned this pull request May 11, 2026

Land analytics-engine PPL integration into main #5430

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize datetime types for unified query API#5408

Normalize datetime types for unified query API#5408
dai-chen merged 1 commit into
opensearch-project:mainfrom
dai-chen:feature/ppl-udt-cleanup

dai-chen commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

dai-chen commented May 6, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

dai-chen commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dai-chen commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Examples

Notes

Related Issues

Check List

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit af2b288)

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

dai-chen commented May 6, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

dai-chen commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dai-chen commented May 6, 2026 •

edited

Loading

github-actions Bot commented May 6, 2026 •

edited

Loading

(Review updated until commit `af2b288`)

github-actions Bot commented May 6, 2026 •

edited

Loading